Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto generate BlockTypeIds & ItemTypeIds #6313

Open
wants to merge 20 commits into
base: minor-next
Choose a base branch
from
Open

Conversation

dktapps
Copy link
Member

@dktapps dktapps commented Mar 29, 2024

Introduction

Currently, implementing new items requires manual changes to both VanillaItems and ItemTypeIds.
The same is true for VanillaBlocks and BlockTypeIds.

This PR removes VanillaItems dependency on the ItemTypeIds::* constants and VanillaBlocks on BlockTypeIds::* constants.
This permits the type IDs to be defined by VanillaItems and VanillaBlocks dynamically, and then regenerating BlockTypeIds and ItemTypeIds is just an afterthought.

This change also makes the code of registering new blocks & items far less obnoxious to read and write, since the information is repeated less times, and the code isn't as wide.

Problems

Adding a new block or item anywhere except right at the end of the setup() function will cause type IDs of existing blocks & items to change. This causes a few issues:

  • Messy diffs (resolved by making const values relative to previous)
  • block_factory_consistency_check can't properly detect changes anymore (needs to be rethought or possibly deleted)
  • Forgetting to regenerate the type IDs could cause wacky behaviour of vanilla features during local testing (automated tests will ensure this doesn't happen on production)

I haven't figured out how to deal with these issues yet. Perhaps the type IDs could be derived from the block type name or something of that nature (but this would be a bit more complicated to implement).

Despite these issues, I think this change is probably still worth making, for the reduction in tedious bullshit when implementing new blocks & items.

Relevant issues

Partially addresses #6311

Changes

API changes

None

Behavioural changes

None

Backwards compatibility

Values of type ID constants have changed. Plugins should not be storing these (as per the giant warning at the top of the files), but this will no doubt catch someone off guard anyway.

Follow-up

Consider if we can get rid of the type ID constants entirely. Perhaps redesigning VanillaBlocks might provide a way to do this.

I've considered the possibility of using enum objects instead of type IDs, which would have various benefits.

Tests

CI tests are sufficient

I realized not long after 5.0.0 release that we could simply use incrementing IDs in VanillaBlocks/VanillaItems (in the same way that plugins have to),
and just generate constants for BlockTypeIds/ItemTypeIds from there. This reduces the number of manual needed to implement new blocks and items, since the autogen can be automated when modifying VanillaBlocks/VanillaItems.

Longer term, we should probably explore getting rid of these IDs altogether. I added them to ensure we had a way to compare item types without cloning items from VanillaBlocks/VanillaItems, but in practice this is likely just premature optimization thinking carried forwards from the old ID system, as pointed out by @jasonw4331.

For the duration of PM5 support, however, this will now have to be maintained, as we can't remove them before PM6.
this makes vanillaBlocks less ugly, and also removes a manual step when adding new blocks.
@dktapps dktapps added Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Mar 29, 2024
this stops the entire file from changing when adding new blocks.
the diff will instead look something like this:

@@ -713,7 +722,8 @@ final class BlockTypeIds{
        public const CHERRY_DOOR = self::CHERRY_STAIRS + 1;
        public const CHERRY_BUTTON = self::CHERRY_DOOR + 1;
        public const CHERRY_PRESSURE_PLATE = self::CHERRY_BUTTON + 1;
-       public const CHERRY_SIGN = self::CHERRY_PRESSURE_PLATE + 1;
+       public const CHERRY_TRAPDOOR = self::CHERRY_PRESSURE_PLATE + 1;
+       public const CHERRY_SIGN = self::CHERRY_TRAPDOOR + 1;
        public const CHERRY_WALL_SIGN = self::CHERRY_SIGN + 1;
        public const CAULDRON = self::CHERRY_WALL_SIGN + 1;
        public const WATER_CAULDRON = self::CAULDRON + 1;
@dktapps dktapps added Category: Dev Workflow Changes dev experience working with PocketMine-MP and removed Category: Core Related to internal functionality labels Mar 29, 2024
we don't actually care about the specific values, only whether all the blocks and their states have been correctly registered.
I'd prefer to track all of the state data permutations, but the APIs for that are private, so tracking the number of permutations will have to suffice (this should be good enough to detect bugs anyway, and also takes way less space).
we can't rely on BlockTypeIds to look up the ID name if the type IDs aren't consistent to begin with...
I don't like using VanillaBlocks for this (we might have multiple source registries in the future, e.g. if we decide to pull the education edition blocks into their own registry), but it doesn't seem like we have a choice.
For now this is valid anyway.
@dktapps
Copy link
Member Author

dktapps commented Mar 30, 2024

Only remaining issue is how to avoid wacky runtime bugs when type IDs get changed due to adding new things

PHPUnit tests will ensure that we don't push broken type IDs to a release, but it could still cause some WTFs in local testing and I'm not sure what should be done to avoid that

in the longer term we really need to find a better way to define these, but without breaking BC, this is the best we can do.
technically there's no reason why the first reserved ID can't be used directly,
but allowing this would require special conditions in the unit tests, so this fix is easier.
@dktapps
Copy link
Member Author

dktapps commented Mar 30, 2024

It seems that longer term, it would make more sense to use strings for type IDs instead of integers. The names used for VanillaBlocks and VanillaItems could be used directly as type IDs.

We'd more or less end up with the same ID system that Minecraft has natively (however, we wouldn't be able to use minecraft: IDs directly since the vanilla IDs don't directly map onto PM blocks due to differences in implementation, such as dynamic colour and other nice APIs that avoid Mojang inconsistencies.)

This would have a few benefits

  • No need for generating anything
  • Easier to transmit between threads (no need to manually claim IDs on every thread)
  • Far easier for custom blocks to avoid collisions
  • Less variables needed for registering new blocks

Downsides:

  • Plugins would be able to hardcode IDs again
  • Plugins would be able to make assumptions about the structure of ID values

We would have to dynamically compress these string IDs to integers at runtime for storing on chunks, but that would be far cleaner than the clusterfuck we currently have.

However, with all that said, we won't be able to do a change like this until PM6 due to the BC breaks required.

auto incrementing IDs are a big pain and don't offer any obvious advantages.
In PM6, it's looking like using strings for type IDs directly will be the logical course of action
However, we can still get some benefit in PM5 from breaking the hard cycle here, and this makes it a bit less inconvenient to implement new things.
@dktapps
Copy link
Member Author

dktapps commented Nov 10, 2024

Probably makes sense to separate the reflection type ID linking from this PR, since that's useful by itself and doesn't change anyone's workflow. Autogenerating type IDs doesn't have such an obvious benefit.

@dktapps dktapps added the Multiple changes PR contains multiple changes and requires separation label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Dev Workflow Changes dev experience working with PocketMine-MP Multiple changes PR contains multiple changes and requires separation Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants